Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent false test failures caused by promise rejections handled asynchronously #14110

Merged
merged 18 commits into from
Jun 21, 2023

Conversation

stekycz
Copy link
Contributor

@stekycz stekycz commented Apr 25, 2023

Summary

TL;DR #6028

I am experiencing a failing test even though it is fine. I create a promise but await it later (asynchronously). The test is failing because it tests a negative condition that rejects.

I believe Jest should register rejectionHandled listener to handle those cases.

Test plan

I have added integration tests to cover all cases I was able to think of. The first commit basically maps the current situation and the following one shows what is actually changed thanks to usage of snapshots. Feel free to suggest other cases I should add.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 25, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: stekycz / name: Martin Štekl (9337d1e)

@netlify
Copy link

netlify bot commented Apr 25, 2023

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b775b19
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/6470b0044a689b000800ee73
😎 Deploy Preview https://deploy-preview-14110--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@SimenB
Copy link
Member

SimenB commented Apr 25, 2023

Ooh, very cool! I didn't know about rejectionHandled. Could you add some integration tests as well?

@stekycz stekycz force-pushed the stekycz-jest-handled-rejection branch from 9337d1e to 67966d7 Compare April 25, 2023 20:16
@stekycz
Copy link
Contributor Author

stekycz commented Apr 25, 2023

@SimenB I have created integration tests. They actually showed me my original implementation did not work as intended so I reworked it. I am open for additional suggestions.

…jectionHandled event only

Awaiting for the next turn is potentially conflicting with fake timers usage.
Legacy fake timers execute original setImmediate in its fake implementation. I believe removing that behaviour would cause more harm than failing the test here that seems to be synthetic anyway.

test('w/o event loop turn after rejection', () => {
Promise.reject(new Error('REJECTED'));
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failures of this test made me update other tests in this PR. I have to force event loop to make a single turn to catch this as unhandled promise rejection.


await promisify(setTimeout)(0);

await expect(promise).rejects.toThrow(/REJECTED/);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to force event loop to make one turn at least when there is an unhandled rejection processed before. Otherwise, this test would be still failing because rejectionHandled needs one event loop turn to be processed. I could make this when there is at least one unhandled rejection before to not modify tests mentioned in the previous comment. However, I would hide those consequences only. Those cases can still happen IMO in situations like this one.

packages/jest-circus/src/unhandledRejectionHandler.ts Outdated Show resolved Hide resolved
packages/jest-types/src/Circus.ts Show resolved Hide resolved
packages/jest-types/src/Circus.ts Show resolved Hide resolved
@@ -11,5 +11,5 @@ test('require after done', () => {
const double = require('../');

expect(double(5)).toBe(10);
}, 0);
}, 100);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are 0 still detected? If not, we should try to set some other flag so we can detect this case immediately after tests are done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they are detected. That is why I had to update this test (and the other one as well).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's still detected I'm not 100% sure why this had to change? Is the error just different?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I miss-understood originally. Unfortunately 0 are not detected after my changes. Possibly even 1 or other small count if ms are not detected. It is cased by the extra event loop turnaround I need to ensure detection of handled and unhandled rejections.

Let me think a bit about the possible solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a deep loop into both cases (jest environment method access after teardown and late require) and both seems fragile already.

  1. Any async event handler can break it. No matter what the even handler would do, anytime it takes at least one even-loop turnaround, this gets broken. More even-loop turnarounds in an async event handler, the more "suppressed" errors like this. Both errors are thrown on runtime level when the circus is out of game already. The runtime assumes the the time between a test and tearing runtime down is strictly sync (which may not be).
  2. "jest environment method access after teardown" validates a single timers method access only. All other Jest environment methods usages are not reported at all, even on the current main branch.

I introduced the first truly async event handler. Therefore, the detection has changed because of the first point.

I was thinking about a new flag, however, I would need to make a hard choice as a user between the following variants:

A)

  • I can keep the current behaviour detecting late require as well as late global method usage.
  • I won't be able to detect unhandled rejections when the promise rejection happens in the same event loop turn as the test ends.
  • I would still get false failures for unhandled rejections when the rejected promise handling happens in the same event loop turn as the test ends.
  • I would still get global unhandled rejection error in case there is a floating promise being rejected way after its creation (e.g. when tearing down the env).

B)

  • I get all promise rejections error accurately (except rejections happening way after the promise creation).
  • I won't get reported on late requires nor late global method usage when it happens within the same event loop turn as the test ends (as it is still considered as part of the test).

I would prefer variant B) as you would probably guess. What is the reasoning behind those checks though?

I think there is a variant C) though. It would mean those 2 specific conditions get changed so they are detect on circus level rather than on runtime level. However, I am not sure how feasible that.

I was also thinking about variant D) - capturing all pending timers, leaving there the one waiting for the next even loop turn and the reapply the remaining timers. However, it feels hacky and I was not able to think of a way for other envs than Node.js (and even that is questionable).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging! I think your change is fine considering the tradeoffs.

I guess what we could do was signalling from Circus that we are done running tests once the teardown event fires, at which point the runtime would start rejecting new requires etc. even though it hasn't started its own teardown. Might not really be feasible in the current architecture, tho?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually got the same idea in the evening but I did not have time to look into it yet 😃 I will give it a shot

Copy link
Contributor Author

@stekycz stekycz May 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I have the solution.

@@ -208,6 +208,7 @@ export default class Runtime {
private readonly esmConditions: Array<string>;
private readonly cjsConditions: Array<string>;
private isTornDown = false;
private isInsideTestCode: boolean | undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am using value undefined as a sign that no test started yet so Jest can still use requires of modules prior running the test module (e.g. requiring the test module itself).

@@ -2169,6 +2193,11 @@ export default class Runtime {
);
process.exitCode = 1;
}
if (this.isInsideTestCode === false) {
throw new ReferenceError(
'You are trying to access a property or method of the Jest environment outside of the scope of the test code.',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am open for suggestions about wording of both kinds of errors.

@stekycz stekycz force-pushed the stekycz-jest-handled-rejection branch from 0681959 to b412305 Compare May 26, 2023 12:05
@stekycz
Copy link
Contributor Author

stekycz commented Jun 8, 2023

@SimenB Is there anything remaining? What can I do to make this land and be in the next release? Thanks

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works for me, thanks! And thanks for your patience

@SimenB SimenB merged commit 57e1d4e into jestjs:main Jun 21, 2023
@stekycz stekycz deleted the stekycz-jest-handled-rejection branch June 21, 2023 10:13
@stekycz
Copy link
Contributor Author

stekycz commented Jun 21, 2023

@SimenB Perfect! Thank you

@SimenB
Copy link
Member

SimenB commented Jul 4, 2023

@juan-fernandez
Copy link

thanks for the contribution, @stekycz. We're having some issues with bad tests (see #14302) and the new isInsideTestCode attribute. While the end goal would be to fix those tests, we currently can only fix them at once before being able to benefit from bumping to 29.6.0 (which has some bugfixes we need). Could some of these ReferenceErrors (such as https://github.com/jestjs/jest/pull/14110/files#diff-c0d5b59e96fdc7ffc98405e8afb46d525505bc7b1c24916b5c8482de5a186c00R1508-R1510) be warnings instead? At least until (maybe) a major release?

@SimenB
Copy link
Member

SimenB commented Jul 6, 2023

Hmm, might be a good idea to defer this to a major release, yeah.

I can revert, release and prepare a PR for v30 later today

@SimenB
Copy link
Member

SimenB commented Jul 6, 2023

https://github.com/jestjs/jest/releases/tag/v29.6.1

@stekycz
Copy link
Contributor Author

stekycz commented Jul 10, 2023

@SimenB Is there something I can help with making it to v30?

@SimenB
Copy link
Member

SimenB commented Jul 10, 2023

@stekycz if you can open up a new PR essentially just reverting my revert, that'd be lovely! 🙂 I'll add it to the milestone for Jest 30 so I don't forget it

stekycz added a commit to stekycz/jest that referenced this pull request Jul 10, 2023
@stekycz
Copy link
Contributor Author

stekycz commented Jul 10, 2023

@SimenB No problem! Here it is #14315

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants